docker-env: change --ssh-host login as root#22577
docker-env: change --ssh-host login as root#22577bhavyaBeliever wants to merge 2 commits intokubernetes:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bhavyaBeliever The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @bhavyaBeliever. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Can one of the admins verify this patch? |
db30f7c to
6f1e645
Compare
851aea9 to
73adedf
Compare
87536a1 to
73adedf
Compare
| } | ||
|
|
||
| if sshHost == true { | ||
| ec.username = "root" |
There was a problem hiding this comment.
I am curious why root has to be set if "sshot"? wouldn't it work with docker user?
and also have u tried it on both VM and Docker driver?
There was a problem hiding this comment.
Yeah It works for the docker user for docker container-runtime, @afbjorklund mentions in the issue that this doesn't work for podman and containerd. The current implementation for containerd uses nerdctl.sock.
Yes we have tested for this for docker driver by making local KIC image with docker and containerd runtime. Similarly we built an ISO file and tested the changes on kvm2 driver.
There was a problem hiding this comment.
Pull request overview
This PR updates minikube’s SSH and docker-env behavior so that the --ssh-host flow and related components support logging in as root instead of docker, aligning better with container runtimes’ default expectations. It updates the kicbase image, ISO automount script, nerdctld startup, and docker-env SSH configuration to provision and use root’s SSH keys and DOCKER_HOST.
Changes:
- Extend KIC SSH preparation and the kicbase image to provision
/root/.ssh/authorized_keys, enabling root SSH access with the same public key as thedockeruser. - Update the ISO automount script to unpack userdata SSH keys into
/root/.sshin addition to/home/docker/.ssh. - Adjust
docker-envandstartNerdctldso--ssh-hostuses therootuser and attempts to ensureDOCKER_HOST=unix:///var/run/nerdctl.sockis set for both docker and root users (with a small bug in the root check condition).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/drivers/kic/kic.go | Copies the generated SSH public key into both /home/docker/.ssh/authorized_keys and /root/.ssh/authorized_keys, and sets ownership, enabling SSH as root in KIC containers. |
| deploy/kicbase/Dockerfile | Ensures /root/.ssh exists in the kicbase image so that root’s authorized_keys can be installed cleanly. |
| deploy/iso/minikube-iso/package/automount/minikube-automount | Mirrors the existing userdata SSH key unpacking for /home/docker into /root, setting root ownership to enable root SSH on the ISO-based nodes. |
| cmd/minikube/cmd/start.go | Extends startNerdctld to check and, if needed, inject DOCKER_HOST=unix:///var/run/nerdctl.sock into both docker and root bash environments, but currently uses the non-root check result when deciding whether to update /root/.bashrc. |
| cmd/minikube/cmd/docker-env.go | Forces SSH connections to use username = "root" when --ssh-host is active and extends appendKnownHelper to run for both containerd and Docker runtimes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
medyagh
left a comment
There was a problem hiding this comment.
lets add Before/After for Containerd/Crio as well
| @@ -2095,8 +2095,27 @@ func startNerdctld(options *run.CommandOptions) { | |||
|
|
|||
| // set up environment variable on remote machine. docker client uses 'non-login & non-interactive shell' therefore the only way is to modify .bashrc file of user 'docker' | |||
| // insert this at 4th line | |||
There was a problem hiding this comment.
Thanks for suggestion: kindly check the updated code
| return fmt.Errorf("create pubkey assetfile : %w", err) | ||
| } | ||
|
|
||
| f2, err := assets.NewFileAsset(d.GetSSHKeyPath()+".pub", "/root/.ssh/", "authorized_keys", "0644") |
There was a problem hiding this comment.
I think the right place might be provisioenr package
There was a problem hiding this comment.
Thank you for suggestion. Please check the implementation did here
medyagh
left a comment
There was a problem hiding this comment.
lets also add before/after for VM drivers as well
11ee157 to
da2b0bd
Compare
I have added for containerd with docker driver, for other cases |
|
/assign @medyagh |
|
lets try another before/after with exacttly explaining what was NOT good before and what is Better now.... |
The thing which was not good was that the docker user was accessing docker.sock illegally (just a workaround). |
Before for KIC driver for docker-runtime:
After:
Before For VM (kvm2) driver for docker-runtime:
After:
Before for containerd runtime:
After:
Fixes #22360